Rmt: Improve codegen for enable_listen_interrupt #2960
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
such that they reduce to a single load/modify/store operation
This iterator generates relatively complex code, which is not reliably reduced to the intended simple memory access, even if the `events` argument is constant at compile time. Rather, the loop might be retained, which is inefficient and leads to much larger code. Tested on riscv using cargo objdump, with the following cargo profile [profile.release] codegen-units = 1 debug = true debug-assertions = false incremental = false lto = 'fat' opt-level = 3|'s' overflow-checks = false
wisp3rwind
changed the title
|
Oh hmm that's unexpectedly significant, thanks! |
bugadani
approved these changes
Jan 16, 2025
bjoernQ
approved these changes
Jan 16, 2025
6 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thank you for your contribution!
We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:
Submission Checklist 📝
cargo xtask fmt-packagescommand to ensure that all changed code is formatted correctly.CHANGELOG.mdin the proper section.Extra:
Pull Request Details 📖
Description
By chance, I've noticed that the
enable_listen_interruptmethods of the RMT driver generates quite bloated code atopt_level: 's'. This seems to happen because theEnumSetiterator is too complicated to be optimized away at this optimization level (it is foropt_level: 2|3).This converts to simple conditionals instead of a loop/match construct in order to help the compiler in generating better code.
I didn't add a changelog entry since there is no user-visible change; not sure that there should be one?
Testing
embassy_rmt_rxandembassy_rmt_txexamples compile for riscv architectures. I don't have an xtensa toolchain installed to easily test that architecture.Here are some code samples of the resulting rmt async interrupt handler obtained via
cargo objdump --release --bin async_main -- -dfor esp32c3 (but the issue also shows up for usage of this method outside of interrupt handler).enumset loops (old code), `opt_level: 3`: code optimized as expected
enumset loops (old code), `opt_level: 's'`: very sub-optimal code: looping over EnumSet fields, using a table in `.rodata`
conditionals (new code), `opt_level: 's'`: code optimized as expected
conditionals (new code), `opt_level: 3`: code optimized as expected, signaling the waker also inlined